Adding RT-1.36: AIGP feature support test#5375
Adding RT-1.36: AIGP feature support test#5375ASHNA-AGGARWAL-KEYSIGHT wants to merge 1 commit intoopenconfig:mainfrom
Conversation
Pull Request Functional Test Report for #5375 / 3c7ca13Virtual Devices
Hardware Devices
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive test coverage for the BGP AIGP (Accumulated IGP Metric) feature, ensuring correct attribute propagation and policy application across different network instances. Additionally, it refactors and extends internal test infrastructure to support new configuration requirements and device-specific deviations, while also refining existing ISIS graceful restart tests for better reliability. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new test suite for the BGP Accumulated IGP Metric (AIGP) feature and updates ISIS Graceful Restart tests with improved telemetry checks. It also extends internal configuration plugins and deviations to support Arista-specific CLI fallbacks for BGP policies, interface encapsulation, and sub-interface formatting. The review feedback identifies a misleading error message in the RIB validation logic, a bug in a test case log format that repeats the test name instead of the description, and the presence of leftover debug logs that should be removed.
| gotAIGP := attrSet.GetAigp() | ||
| if gotAIGP != wantAIGP { | ||
| t.Errorf("fail: aigp for neighbor %s in NI %s: want %d, got %d", nbrAddr, ni, wantAIGP, gotAIGP) | ||
| t.Errorf("OC not supported for AIGP attribute in BGP RIB") |
There was a problem hiding this comment.
This error message is misleading. Since gotAIGP was successfully retrieved using OpenConfig on line 1023, the claim that OpenConfig is not supported is contradictory. If the metric comparison fails, the preceding t.Errorf on line 1026 already provides the necessary failure details. Additionally, consider using t.Fatalf instead of t.Errorf if a failure here makes subsequent test steps meaningless.
References
- In tests, t.Fatalf is preferred over t.Errorf when a failure makes subsequent test steps meaningless, as this fails fast and reduces overall test execution time.
| // Run the test cases. | ||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Logf("Description: %s - %s", tc.name, tc.name) |
There was a problem hiding this comment.
| t.Log(dut1loopback10.loopbackIntfName) | ||
| t.Log(dut2loopback10.loopbackIntfName) |
bdf443e to
3c7ca13
Compare
| gnmi.Update(t, dut, gnmi.OC().Config(), d) | ||
| } | ||
|
|
||
| // VlanClientEncapsulationParams configures vlan encapsulation params |
There was a problem hiding this comment.
i don't have a suggestion of where to move the diff in this file, but none of the additions are mpls related, so doesn't belong in this file.
|
|
||
| func VlanClientEncapsulation(t *testing.T, batch *gnmi.SetBatch, dut *ondatra.DUTDevice, params VlanClientEncapsulationParams) { | ||
| if deviations.VlanClientEncapsulationOcUnsupported(dut) { | ||
| cli := "" |
There was a problem hiding this comment.
cli needs to be conditional on a dut.vendor() check, right?
| } | ||
| helpers.GnmiCLIConfig(t, dut, cli) | ||
| } else { | ||
| // OC is not available |
There was a problem hiding this comment.
what does this mean? are oc paths not defined?
| } | ||
|
|
||
| func VlanClientEncapsulation(t *testing.T, batch *gnmi.SetBatch, dut *ondatra.DUTDevice, params VlanClientEncapsulationParams) { | ||
| if deviations.VlanClientEncapsulationOcUnsupported(dut) { |
There was a problem hiding this comment.
not sure this needs to be a devition as this is used to workaround testbed contraints. it's not functionality germane to the test.
| } | ||
|
|
||
| // VerifyDUTBGPEstablishedForVRF verifies on DUT BGP peer establishment | ||
| func VerifyDUTBGPEstablishedForVRF(t *testing.T, dut *ondatra.DUTDevice, ni string, duration ...time.Duration) { |
There was a problem hiding this comment.
this is identincal to VerifyDUTBGPEstablished expect for the vrf name, please refactor
| if enable { | ||
| cliConfig += fmt.Sprintf(` | ||
| neighbor %s aigp-session | ||
| `, nbrIp) | ||
| } else { | ||
| cliConfig += fmt.Sprintf(` | ||
| no neighbor %s aigp-session | ||
| `, nbrIp) | ||
| } |
There was a problem hiding this comment.
if !enable {
maybeNo = "no "
}
cliConfig += fmt.Sprintf(%sneighbor %s aigp-session, maybeNo, nbrIp)
| if deviations.AIGPRouteMetricNotSupported(dut) { | ||
| switch dut.Vendor() { | ||
| case ondatra.ARISTA: | ||
| cliConfig := fmt.Sprintf(`route-map %s statement %s `, params.PolicyName, params.StatementName) |
There was a problem hiding this comment.
we should be using RCF
Readme: https://github.com/toadeniy-g/featureprofiles/blob/main/feature/bgp/otg_tests/aigp_test/README.md
Raised an issue for the test not passing completely: https://partnerissuetracker.corp.google.com/issues/505782544